Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

FIX: Allow Copy/Paste between windows (ISX-1823). #1844

Open
wants to merge 23 commits into
base: develop
Choose a base branch
from
Open

FIX: Allow Copy/Paste between windows (ISX-1823). #1844

wants to merge 23 commits into from
+64 −17

Conversation

graham-huws
Copy link
Collaborator

@graham-huws graham-huws commented Feb 15, 2024 •
edited
Loading

Description

Fix for ISX-1823.

Changes made

This was previously not working because the Paste command would only apply to the relevant Action Maps/Actions List/Tree, so if neither of those were focused, nothing would happen. We now also listen for Paste at the top level of the element hierarchy and send paste commands from there.

Notes

Issues:
• Undoing adding an Action Map/Action leaves the list unselected. Pasting at that point fails because the state thinks there's still a valid item selected. Mentioned here, and should be added as a separate bug.
• Currently won't work if the window is tabbed to - user has to click somewhere in the window. Think I'll need to ask UITK team about this.

Checklist

Before review:

  • Changelog entry added.
    • Explains the change in Changed, Fixed, Added sections.
    • For API change contains an example snippet and/or migration example.
    • FogBugz ticket attached, example ([case %number%](https://issuetracker.unity3d.com/issues/...)).
    • FogBugz is marked as "Resolved" with next release version correctly set.
  • Tests added/changed, if applicable.
    • Functional tests Area_CanDoX, Area_CanDoX_EvenIfYIsTheCase, Area_WhenIDoX_AndYHappens_ThisIsTheResult.
    • Performance tests.
    • Integration tests.
  • Docs for new/changed API's.
    • Xmldoc cross references are set correctly.
    • Added explanation how the API works.
    • Usage code examples added.
    • The manual is updated, if needed.

During merge:

  • Commit message for squash-merge is prefixed with one of the list:
    • NEW: ___.
    • FIX: ___.
    • DOCS: ___.
    • CHANGE: ___.
    • RELEASE: 1.1.0-preview.3.

@graham-huws graham-huws force-pushed the ISX-1823-cut-copy-between-windows branch from 94d27e7 to 8944d39 Compare February 15, 2024 00:46
@graham-huws graham-huws force-pushed the ISX-1823-cut-copy-between-windows branch from 8944d39 to 4d21dbf Compare February 15, 2024 11:41
@graham-huws graham-huws marked this pull request as ready for review February 15, 2024 11:42
ekcoh
ekcoh reviewed Feb 15, 2024
View reviewed changes
Packages/com.unity.inputsystem/CHANGELOG.md Outdated Show resolved Hide resolved
Packages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/CopyPasteHelper.cs Outdated
@@ -288,13 +292,14 @@ private static void PasteAction(SerializedProperty arrayProperty, string jsonToI
private static int PasteBindingOrComposite(SerializedProperty arrayProperty, string json, int index, string actionName, bool createCompositeParts = true)
{
var pastePartOfComposite = IsPartOfComposite(json);
if (index > 0)
if (index > 0 && arrayProperty.arraySize > 0 && index - 1 < arrayProperty.arraySize)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am confused about an index that must be greater than zero and at least two elements from the end of the anway? Would recommend adding an inline comment if this is correct but some kind of special case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This somewhat doubled up from merging develop - should be clearer now :)

Packages/com.unity.inputsystem/InputSystem/Editor/UITKAssetEditor/Views/CopyPasteHelper.cs Show resolved Hide resolved
@ekcoh
Copy link
Collaborator

ekcoh commented Feb 15, 2024

As a side-note, when #1834 lands there will exist an interesting scenario for this functionality when the same asset is open simultaneously in two windows, both in Project Settings and in a free-floating Input Action Editor.

@stefanunity stefanunity requested review from stefanunity and removed request for Pauliusd01 February 19, 2024 10:07
stefanunity
stefanunity approved these changes Feb 21, 2024
View reviewed changes
Copy link
Collaborator

@stefanunity stefanunity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finished for now. Two bugs not yet fixed are:

  1. BUG: trying to paste an action into empty map throws error:
    --ArgumentException: '0' cannot be greater than -1.
  2. BUG: trying to paste a modifier into empty map throws errror
    --NullReferenceException: Object reference not set to an instance of an object

Once they are fixed it looks good.

@ritamerkl ritamerkl requested a review from Pauliusd01 February 22, 2024 11:45
@graham-huws
Copy link
Collaborator Author

Finished for now. Two bugs not yet fixed are:

1. BUG: trying to paste an action into empty map throws error:
   --ArgumentException: '0' cannot be greater than -1.

2. BUG: trying to paste a  modifier into empty map throws errror
   --NullReferenceException: Object reference not set to an instance of an object

Once they are fixed it looks good.

Thanks @stefanunity - I believe these should both be fixed now, can you confirm?

@Pauliusd01
Copy link
Collaborator

Pauliusd01 commented Feb 22, 2024 •
edited
Loading

Unity_2024-02-22_14-11-34.mp4

Cutting and pasting into an empty asset somehow "resurrects" all of the previously deleted actions. (I do deselect ProjectWideActions to confirm the auto save but that does not seem to help)

@stefanunity
Copy link
Collaborator

Thanks @stefanunity - I believe these should both be fixed now, can you confirm?

Both are fixed now.

@ekcoh
Copy link
Collaborator

ekcoh commented Feb 23, 2024

The bug called out @Pauliusd01 doesn't really make any sense to me but should be fixed. Where is this coming from if not the copy buffer?

Pauliusd01
Pauliusd01 requested changes Feb 28, 2024
View reviewed changes
Copy link
Collaborator

@Pauliusd01 Pauliusd01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updating status - waiting for fixes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ritamerkl ritamerkl ritamerkl left review comments

@ekcoh ekcoh ekcoh left review comments

@Pauliusd01 Pauliusd01 Pauliusd01 requested changes

@stefanunity stefanunity stefanunity approved these changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@graham-huws @ekcoh @Pauliusd01 @stefanunity @ritamerkl @lyndon-unity